server/v2/stf/core_event_service.go (1)

40-53: Consider adding validation and documentation.

While the implementation is correct, consider these improvements:

1. Add validation for nil message
2. Document the delayed marshalling behavior in the method comment

 // Emit emits an typed event that is defined in the protobuf file.
-// In the future these events will be added to consensus.
+// The event data is marshalled lazily when the Data() function is called,
+// improving performance by avoiding unnecessary marshalling operations.
 func (em *eventManager) Emit(tev transaction.Msg) error {
+	if tev == nil {
+		return fmt.Errorf("cannot emit nil event")
+	}
+
 	event := event.Event{

server/v2/cometbft/config.go (1)

45-48: Consider adding validation for event strings

While the implementation looks good, consider adding validation for the IndexABCIEvents entries to ensure they follow the expected format of {eventType}.{attributeKey}. This would help catch configuration errors early.

Example validation function:

func validateEventString(event string) error {
    parts := strings.Split(event, ".")
    if len(parts) != 2 {
        return fmt.Errorf("event must be in format {eventType}.{attributeKey}, got %s", event)
    }
    if parts[0] == "" || parts[1] == "" {
        return fmt.Errorf("both eventType and attributeKey must be non-empty")
    }
    return nil
}

tools/confix/data/v2-app.toml (3)

24-28: Enhance documentation for event indexing configuration

The event indexing configuration options are crucial for the new delayed marshalling feature, but the documentation could be more detailed:

1. Add examples of event types in the index-abci-events description
2. Explain the performance implications of disable-abci-events
3. Document the relationship between these two settings

-# index-abci-events defines the set of events in the form {eventType}.{attributeKey}, which informs CometBFT what to index. If empty, all events will be indexed.
+# index-abci-events defines the set of events in the form {eventType}.{attributeKey}, which informs CometBFT what to index.
+# If empty, all events will be indexed. Example: ["message.sender", "transfer.recipient"]
 index-abci-events = []

-# disable-abci-events disables the ABCI event indexing. It is useful when relying on the indexer for event indexing.
+# disable-abci-events disables ABCI event marshalling, improving performance when using the indexer for event querying.
+# Note: When enabled, events will only be available through the indexer and not directly via ABCI responses.
 disable-abci-events = false

---

11-20: Review security implications of default network bindings

The default CometBFT RPC server address is bound to localhost, which is secure. However, consider:

1. Adding a warning about security implications of changing the bind address
2. Documenting the relationship between standalone mode and other settings

-# address defines the CometBFT RPC server address to bind to.
+# address defines the CometBFT RPC server address to bind to.
+# WARNING: Binding to 0.0.0.0 or public addresses may expose the RPC server to public networks.
+# It's recommended to use localhost or private addresses in production.
 address = 'tcp://127.0.0.1:26658'

 # standalone starts the application without the CometBFT node. The node should be started separately.
+# When enabled, certain RPC server settings like 'address' and 'transport' may not apply.
 standalone = false

---

135-140: Improve telemetry configuration examples

The global labels configuration would benefit from more comprehensive examples showing different use cases:

 # GlobalLabels defines a global set of name/value label tuples applied to all metrics emitted using the wrapper functions defined in telemetry package.
-#  Example:
-#   [["chain_id", "cosmoshub-1"]]
+# Examples:
+#   [
+#     ["chain_id", "cosmoshub-1"],
+#     ["network", "mainnet"],
+#     ["version", "v1.0.0"]
+#   ]
 global-labels = []

server/v2/cometbft/abci.go (1)

108-113: Consider error handling optimization.

While the conditional handling of ABCI events is good, we can optimize the code by avoiding the empty slice allocation when events are disabled.

-events := make([]abci.Event, 0)
+var events []abci.Event
 if !c.cfg.AppTomlConfig.DisableABCIEvents {
+    events = make([]abci.Event, 0)
     events, err = intoABCIEvents(resp.Events, c.indexedABCIEvents)
     if err != nil {
         return nil, err
     }
 }

server/v2/cometbft/utils.go (4)

76-77: Avoid grouping parameters of the same type; specify types for each parameter.

For clarity and to comply with the Uber Go Style Guide, please specify the type for each parameter individually. Grouping parameters can make the code harder to read and understand.

Apply this diff to improve parameter declarations:

 func finalizeBlockResponse(
     in *server.BlockResponse,
     cp *cmtproto.ConsensusParams,
     appHash []byte,
     indexSet map[string]struct{},
-    disableABCIEvents,
-    debug bool,
+    disableABCIEvents bool,
+    debug bool,
 ) (*abci.FinalizeBlockResponse, error) {

---

119-123: Avoid grouping parameters of the same type; specify types for each parameter.

Similarly, in the intoABCITxResults function, specify the type for each parameter individually to enhance readability.

Apply this diff:

 func intoABCITxResults(
     results []server.TxResult,
     indexSet map[string]struct{},
-    disableABCIEvents, debug bool,
+    disableABCIEvents bool,
+    debug bool,
 ) ([]*abci.ExecTxResult, error) {

---

157-157: Update error message to reflect the operation performed.

The error message should accurately describe the failed operation. Since e.Data() retrieves event data, consider updating the message.

Apply this diff:

-    return nil, fmt.Errorf("failed to marshal event data: %w", err)
+    return nil, fmt.Errorf("failed to retrieve event data: %w", err)

---

175-179: Avoid redeclaring 'err' variable; reuse the existing 'err' variable.

The variable err is already declared in an outer scope. Redeclaring it here is unnecessary and could lead to confusion.

Apply this diff to eliminate the redundant declaration:

 } else {
-    var err error
     attrs, err = e.Attributes()
     if err != nil {
         return nil, err
     }

Configuration used: .coderabbit.yml
Review profile: CHILL

#!/bin/bash
# Description: Check for any remaining instances of the old variable name
# and confirm the new naming convention is used consistently

# Check for any remaining instances of old name
echo "Checking for any remaining instances of 'indexedEvents'..."
rg "indexedEvents" --type go

# Verify new name usage
echo "Verifying usage of new name 'indexedABCIEvents'..."
rg "indexedABCIEvents" --type go

#!/bin/bash
# Description: Check for consistent usage of the new configuration fields

# Check for proper handling of DisableABCIEvents
echo "Checking DisableABCIEvents usage:"
rg -A 5 "DisableABCIEvents"

# Check for proper handling of IndexABCIEvents
echo "Checking IndexABCIEvents usage:"
rg -A 5 "IndexABCIEvents"

# Check for any remaining references to the old IndexEvents
echo "Checking for old IndexEvents references:"
rg "IndexEvents"

#!/bin/bash
# Let's check for any potential conflicts or inconsistencies in event handling

# Check for any references to event indexing in the ABCI implementation
echo "Checking ABCI event handling implementation:"
rg -A 5 "intoABCIEvents"

# Check the server initialization to see how these configs are used
echo "Checking server initialization:"
ast-grep --pattern 'func New[A-Za-z]*Server'

# Check if there are any migration or conversion utilities
echo "Checking migration utilities:"
fd -t f confix

#!/bin/bash
# Search for event consumers in the codebase
echo "Searching for event consumers..."
rg -A 5 "Events.*\[\]abci\.Event" 

# Search for indexer implementations
echo "Searching for indexer implementations..."
rg -A 5 "type.*Indexer.*interface"

#!/bin/bash
# Search for finalizeBlockResponse implementation
echo "Searching for finalizeBlockResponse implementation..."
ast-grep --pattern 'func finalizeBlockResponse($$$)'

# Search for DisableABCIEvents usage
echo "Searching for DisableABCIEvents usage..."
rg -A 5 "DisableABCIEvents"

# Search for event indexing related code
echo "Searching for event indexing code..."
rg -A 5 "indexedABCIEvents"

server/v2/stf/core_event_service.go (1)

40-50: Implementation looks good with room for improvement

The delayed marshalling implementation through a closure aligns well with the performance objectives. However, consider these improvements:

1. The closure could be extracted into a named function for better reusability and testing
2. Consider adding context timeout for the marshalling operation to prevent potential blocking

Here's a suggested refactoring:

 func (em *eventManager) Emit(tev transaction.Msg) error {
 	event := event.Event{
 		Type: gogoproto.MessageName(tev),
-		Data: func() (json.RawMessage, error) {
-			buf := new(bytes.Buffer)
-			jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: true, AnyResolver: nil}
-			if err := jm.Marshal(buf, tev); err != nil {
-				return nil, err
-			}
-
-			return buf.Bytes(), nil
-		},
+		Data: marshalEventData(tev),
 	}
 }

+func marshalEventData(msg gogoproto.Message) func() (json.RawMessage, error) {
+	return func() (json.RawMessage, error) {
+		buf := new(bytes.Buffer)
+		jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: true, AnyResolver: nil}
+		if err := jm.Marshal(buf, msg); err != nil {
+			return nil, err
+		}
+		return buf.Bytes(), nil
+	}
+}

server/v2/cometbft/config.go (1)

48-50: Consider adding validation for event format

The IndexABCIEvents field accepts events in the form {eventType}.{attributeKey}. Consider adding validation to ensure the correct format is provided.

Example validation function:

func validateEventFormat(events []string) error {
    for _, event := range events {
        if !strings.Contains(event, ".") {
            return fmt.Errorf("invalid event format %q: must be in the form {eventType}.{attributeKey}", event)
        }
    }
    return nil
}

server/v2/cometbft/utils.go (4)

80-89: Handle DisableABCIEvents configuration flag correctly

The conditional block checks if !cfg.DisableABCIEvents to determine whether to process events. Using a negative configuration flag can be prone to errors and reduce code clarity.

Consider renaming DisableABCIEvents to EnableABCIEvents for positive logic:

-	if !cfg.DisableABCIEvents {
+	if cfg.EnableABCIEvents {

And adjust the configuration struct accordingly.

---

132-136: Handle DisableABCIEvents in transaction results processing

Similar to the previous comment, the intoABCITxResults function checks if !cfg.DisableABCIEvents. For consistency and clarity, consider applying the same positive logic refactoring here.

Apply this diff:

-	if !cfg.DisableABCIEvents {
+	if cfg.EnableABCIEvents {

---

151-151: Avoid double negatives by renaming indexNone variable

Using double negatives like !indexNone can make the code harder to understand. Consider renaming indexNone to indexEvents or enableIndexing to reflect positive logic, improving readability.

Apply this diff to rename the variable:

-func intoABCIEvents(events []event.Event, indexSet map[string]struct{}, indexNone bool) ([]abci.Event, error) {
+func intoABCIEvents(events []event.Event, indexSet map[string]struct{}, indexEvents bool) ([]abci.Event, error) {

...

-	Index: !indexNone && (index || indexAll),
+	Index: indexEvents && (index || indexAll),

Ensure that all function calls to intoABCIEvents are updated accordingly.

Also applies to: 195-195

---

158-166: Handle JSON unmarshalling errors gracefully

In the event data processing, errors from json.Unmarshal are returned immediately. Consider providing more context in the error messages to aid in debugging.

Enhance error messages as follows:

-	return nil, fmt.Errorf("failed to unmarshal event data: %w", err)
+	return nil, fmt.Errorf("failed to unmarshal event data for event type '%s': %w", e.Type, err)

Configuration used: .coderabbit.yml
Review profile: CHILL

#!/bin/bash
# Description: Verify the implementation of intoABCISimulationResponse function
# and its usage of the new parameters.

# Search for the function definition
ast-grep --pattern 'func intoABCISimulationResponse($_) {
  $$$
}'

# Search for other usages of this function to ensure consistent parameter passing
rg 'intoABCISimulationResponse\(' -A 3

#!/bin/bash
# Let's verify the implementation of intoABCIEvents function and its usage
# since it's called within intoABCISimulationResponse

# Search for the intoABCIEvents function definition
ast-grep --pattern 'func intoABCIEvents($_) {
  $$$
}'

# Search for all usages of intoABCIEvents
rg 'intoABCIEvents\(' -A 3

#!/bin/bash
# Search for event format parsing and validation
rg -A 5 "index-abci-events" --type go
ast-grep --pattern 'func $_(events []string) $_ {
    $$$
}'

#!/bin/bash
# Search for event-related validation or processing code
rg -A 10 "type.*Event" --type go
rg -A 10 "func.*Event" --type go

# Search for any validation related to event format
ast-grep --pattern 'if $_ := strings.Split($event, "."); $_ {
    $$$
}'

# Look for event-related constants or types
rg "EventType|AttributeKey" --type go

#!/bin/bash
# Search for event format validation in indexer package
rg -A 10 "index.*events" indexer/
rg -A 10 "validateEvent" indexer/

# Look for event format parsing in CometBFT config
rg -A 10 "IndexABCIEvents" server/v2/cometbft/

# Search for event format validation tests
rg -A 10 "TestEvent.*Format" --type go

#!/bin/bash
# Search for event format validation in CometBFT code
rg -A 10 "intoABCIEvents" server/v2/cometbft/
rg -A 10 "indexSet" server/v2/cometbft/

# Look for event format parsing functions
ast-grep --pattern 'func $_(events []string) $_ {
    $$$
    strings.Split($_, ".")
    $$$
}'

# Search for event format related tests
rg -A 10 "test.*event.*format" --type go -i

#!/bin/bash
# Search for storage configuration validation
rg -A 10 "func.*Validate" store/v2/root/

#!/bin/bash
# Description: Check for any remaining references to the old configuration key
# Expected: No occurrences of the old key in test configs and server code

# Check test configuration files
echo "=== Test Configuration Files ==="
fd -e toml . tools/confix/data --exec grep -l "index-events" {} \;

# Check server code
echo -e "\n=== Server Code ==="
rg "index-events" server/ --type go

#!/bin/bash
# Find the implementation of finalizeBlockResponse to verify config usage
ast-grep --pattern 'func finalizeBlockResponse($$$) {
  $$$
}'

#!/bin/bash
# Let's try to find the finalizeBlockResponse function using ripgrep
rg -A 20 "func finalizeBlockResponse" 

# Also search for any usage of AppTomlConfig and indexedABCIEvents
rg "AppTomlConfig|indexedABCIEvents" -A 5

-				return nil, err
+				return nil, fmt.Errorf("failed to convert events: %w", err)

server/v2/cometbft/utils.go (1)

73-86: LGTM! Consider enhancing error handling.

The configuration-driven event processing aligns well with the PR objectives for flexible event handling. The conditional check for DisableABCIEvents effectively implements the delayed marshalling feature.

Consider wrapping the error with additional context:

 if err != nil {
-    return nil, err
+    return nil, fmt.Errorf("failed to process ABCI events: %w", err)
 }

Configuration used: .coderabbit.yml
Review profile: CHILL

tools/confix/data/v2-app.toml (2)

18-21: Clarify the relationship between event configuration parameters

The configuration now has three related parameters that affect event handling:

• index-abci-events: Controls which events to index
• disable-index-abci-events: Disables CometBFT event indexing
• disable-abci-events: Disables all ABCI events

Please clarify:

1. The precedence order of these parameters
2. The behavior when conflicting values are set
3. The impact on existing functionality that depends on events

Consider adding validation to:

1. Detect and warn about conflicting settings
2. Ensure graceful degradation of event-dependent features
3. Document the impact on existing event consumers

---

16-21: Document performance implications of event configuration

The new event configuration options can significantly impact system performance. Consider adding comments that:

1. Document the performance impact of indexing all events (default behavior)
2. Provide guidance on selecting events to index based on system requirements
3. Explain the memory and storage implications of different configurations

Add configuration examples and performance implications in comments:

 # index-abci-events defines the set of events in the form {eventType}.{attributeKey}, which informs CometBFT what to index. If empty, all events will be indexed.
+# Example: ["transfer.recipient", "withdraw.validator"] indexes only specific events
+# Note: Indexing all events may impact performance in high-throughput chains
 index-abci-events = []

Configuration used: .coderabbit.yml
Review profile: CHILL